-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
upgrade react 18 #616
base: main
Are you sure you want to change the base?
upgrade react 18 #616
Conversation
Visit the preview URL for this PR (updated for commit f549a18): https://pixiv-charcoal-web--pr616-chore-upgrade-react1-ejxd9fz2.web.app (expires Wed, 05 Feb 2025 11:01:26 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 314b26d3adca98a761c7e4d9922ebb206ff024a0 |
Size Change: +327 B (+0.06%) Total Size: 552 kB
ℹ️ View Unchanged
|
styled-components を 5 にできるならそれのほうがいい |
dd3026d
to
7b7fc6b
Compare
"react-test-renderer": "17.0.2", | ||
"react": "^18.3.1", | ||
"react-dom": "^18.3.1", | ||
"react-test-renderer": "^18.3.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
react-test-rendererはdeprecatedになったので移行したいですね
"@types/warning": "^3.0.0", | ||
"@types/webpack-env": "^1.18.1", | ||
"@vitejs/plugin-react": "^4.3.1", | ||
"jsdom": "^24.1.0", | ||
"react": "^17.0.2", | ||
"react": "^18.3.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
そもそもiconsがreact非依存のほうが正しそうな気もします
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HTMLAttribute 型を使用するために入っているのと storybook の test 用で依存が入っているようです。
pnpm に移行するときにも pacakge の package.json に依存がないと依存先が見つからなくて error になった気もするので devDeps に置いておいてもよいと思ったのですが、node-linker=hoisted
することにして必要ないものはすべて消すっていう運用のほうがいいんですかね?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
root の package.json に入っていれるから消しちゃってもいいのか
packages/icons/package.json
Outdated
@@ -22,13 +22,14 @@ | |||
}, | |||
"devDependencies": { | |||
"@storybook/react": "^8.0.5", | |||
"@types/jest": "^27.4.0", | |||
"@types/react": "^17.0.38", | |||
"@types/dompurify": "^2.3.3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ここはもう消したはずなのでrebaseのミス?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
みすっぽいですね....消しときます
packages/react-sandbox/package.json
Outdated
"react": ">=17.0.0", | ||
"react-dom": ">=16.13.1", | ||
"styled-components": ">=5.1.1" | ||
"react": ">=18.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
17.x引き続きサポートできればしたいです
@@ -18,8 +18,9 @@ export const DefaultLink = React.forwardRef<HTMLAnchorElement, LinkProps>( | |||
} | |||
) | |||
|
|||
type DefaultLinkProps = LinkProps & React.RefAttributes<HTMLAnchorElement> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RefAttributesは17.xになさそうで、breakingを回避できるならしたいかもです
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const Container = styled.div.attrs<Props, ReturnType<typeof styledProps>>( | ||
styledProps | ||
)` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
attrsが消えたことによってpropsが変わっていそう?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
div の attr として context は持ってないのに context を渡していたので修正したようです!(数か月前のこと過ぎてこの修正を調べて推測したことを言っています....)
packages/react-sandbox/src/type.d.ts
Outdated
import { CharcoalTheme } from '@charcoal-ui/theme' | ||
import { CSSProp, DefaultTheme } from 'styled-components' | ||
import type { CSSProp } from 'styled-components' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ここを変えた理由を知りたいです
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
styled-components
を 6 にした際に generic ではなかったので修正したのですが、5 にしたので戻しておきます....!
packages/styled/package.json
Outdated
"react": ">=17.0.0", | ||
"styled-components": ">=5.1.1" | ||
"react": ">=18.0.0", | ||
"styled-components": ">=5.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
意図的にバージョンを下げたのですか?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
すみません!これミスです!
やったこと
動作確認環境
チェックリスト
不要なチェック項目は消して構いません